feat: non-singleton EppoJSClient #166

typotter · 2025-01-31T16:40:41Z

labels: mergeable

Eppo Internal
🎟️ Fixes: FF-3888
📜 Multiple Eppo Clients
Configuration Side-loading

Motivation and Context

The init and getInstance() methods work on a singleton instance of the EppoClient which makes for a generally smooth developer experience given that the Eppo SDK essentially handles dependency management for the dev. This does have its own drawbacks, but those are not particularly relevant here.

There are use cases, however, when a non-singleton instance of the EppoClient is required. One such use case is embedding the Eppo SDK into a library which itself can be included in other applications. If that 3p library shipped with Eppo is integrated into another application that has the Eppo SDK running, there would be undefined behaviour as the SDK cannot handle this use case.

The init method (and class constructors) for EppoClient and subclasses have evolved organically over time, adding new options as new features are added to the clients. The very large options type is beginning to become a little untenable and disorganized so we take the opportunity to clean that up a bit here.

There are other limitations and drawbacks to the current model of instantiating an EppoClient statically and then initializing it when the code calls init including the awkward coupling of needing to wait for init to resolve in order to get a reference to an initialized client. We have an opportunity to decouple initialization and waiting to make for a better DX (in addition to giving the dev full control over managing the EppoClient reference (intrinsically allows for easier mocking in tests and will be consistent with the host applications existing DI approach).

This change must be done without a major version bump and completely preserve the existing singleton API

Description

First, refactor the initialization options by grouping related options and extracting different types for each.
Each option type is then combined back into a type using the existing IClientConfig name (this keeps the change backwards compatible while offering a big win in option clarity)
Move the static reference initialized to non static.
Accept an EppoJSClient instance param instead of accessing though EppoJSClient.instance where applicable
Where the static instance must be accessed, change to getInstance
Allow the EppoJSClient constructor to accept the full options object. When full options are passed, kick off initializing the client. Otherwise, follow the "old" contstructor path
Docs

How has this been documented?

https://linear.app/eppo/issue/FF-3926/[docs]-update-js-client-docs-to-describe-the-non-singleton-client

How has this been tested?

tests

package.json

src/i-client-config.ts

typotter · 2025-01-31T19:39:56Z

src/index.ts

  public static instance = new EppoJSClient({
    flagConfigurationStore,
    isObfuscated: true,
  });
-  public static initialized = false;
+  initialized = false;


move static members to the instance

typotter · 2025-01-31T19:41:48Z

src/index.ts

+    }
+  }
+
+  public waitForReady(): Promise<void> {


new method! 🎉

what does "ready" mean in this context? adding comments is likely to help, also we should always do it for public APIs anyways

renamed to waitForInitialization

waitForConfiguration to be in line with multiplatform's analog PollerThread::waitForConfiguration

src/index.ts

typotter · 2025-01-31T19:42:41Z

src/index.ts

-    if (!EppoJSClient.initialized) {
+  private ensureInitialized() {
+    if (!this.initialized) {
+      // TODO: check super.isInitialized?


EppoClient.initialized also exists, but means something different than it does here.

what does it mean and how is that different than this? consider renaming either if so

the super method checks that all the config stores are initialized while this this.initialized is true when all the initialization workload is done so they are pretty much the same.
I think we can move to deprecate EppoJSClient.initialized and defer to super.isInitialized().

typotter · 2025-01-31T19:43:39Z

src/index.ts

+  /**
+   * Tracks pending initialization. After an initialization completes, the value is removed from the map.
+   */
+  private static initializationPromise: Promise<EppoJSClient> | null = null;
+
+  /**
+   * This method is part of a bridge from using a singleton to independent instances. More specifically, it exists so
+   * that the init method can access the private field, `readyResolver`. It should not be called by any
+   * methods other than the `init` method. There are limited guards in place; the behaviour if called inappropriately
+   * is undefined.
+   *
+   * It also keeps code that relies on internal details of EppoJSClient colocated in the class.
+   *
+   * @internal
+   *
+   * @param client
+   * @param config
+   */
+  static async initializeClient(
+    client: EppoJSClient,
+    config: IClientConfig,
+  ): Promise<EppoJSClient> {
+    validation.validateNotBlank(config.apiKey, 'API key required');
+
+    // If there is already an init in progress for this apiKey, return that.
+    if (!EppoJSClient.initializationPromise) {
+      EppoJSClient.initializationPromise = explicitInit(config, client).then((client) => {
+        // Resolve the ready promise if it exists
+        if (client.readyPromiseResolver) {
+          client.readyPromiseResolver();
+          client.readyPromiseResolver = null;
+        }
+        return client;
+      });
+    }
+
+    const readyClient = await EppoJSClient.initializationPromise;
+    EppoJSClient.initializationPromise = null;
+
+    return readyClient;
+  }
 }


This is the old init method and initialization buffer tracker.
Moved the method to a static member of EppoJSClient so that it can access the readyResolver and notify waitForReady

src/index.ts

typotter · 2025-01-31T19:46:45Z

src/index.ts

 /**
 * Initializes the Eppo client with configuration parameters.
 * This method should be called once on application startup.
 * If an initialization is in process, calling `init` will return the in-progress
 * `Promise<EppoClient>`. Once the initialization completes, calling `init` again will kick off the
 * initialization routine (if `forceReinitialization` is `true`).
+ *
+ *
+ * @deprecated


controversial, I know.
willing to negotiate

Seems like a minor change for the end user if the argument interface stays identical

The @deprecated annotation is here to help push devs to using the new construct + wait paradigm. This method will be removed and the argument interface for the new constructor is slightly different sdkKey vs apiKey

typotter · 2025-01-31T19:46:53Z

src/index.ts

 * @param config - client configuration
 * @public
 */
 export async function init(config: IClientConfig): Promise<EppoClient> {
-  validation.validateNotBlank(config.apiKey, 'API key required');


moved to EppoJSClient

src/index.ts

typotter

Thanks everyone for the great comments and perspectives.

Refactored back to a little closer to where we started; dropped all the dual-constructor/config stuff.

typotter · 2025-02-04T16:25:30Z

src/index.ts

-  public static initialized = false;
+  initialized = false;
+
+  constructor(optionsOrConfig: EppoClientParameters | IClientOptions) {


The constructor is already public, so we needed to keep the EppoClientParameters in the constructor to avoid a major change.

That being said, I moved to a static builder method instead that lets us get away from the dual constructor.

typotter · 2025-02-04T16:29:57Z

src/index.ts

 /**
 * Initializes the Eppo client with configuration parameters.
 * This method should be called once on application startup.
 * If an initialization is in process, calling `init` will return the in-progress
 * `Promise<EppoClient>`. Once the initialization completes, calling `init` again will kick off the
 * initialization routine (if `forceReinitialization` is `true`).
+ *
+ *
+ * @deprecated


The @deprecated annotation is here to help push devs to using the new construct + wait paradigm. This method will be removed and the argument interface for the new constructor is slightly different sdkKey vs apiKey

src/index.ts

typotter · 2025-02-05T04:30:50Z

src/i-client-config.ts


-  /** Configuration settings for the event dispatcher */


typotter · 2025-02-05T04:46:31Z

src/index.ts

+    return client;
+  }
+
+  async init(config: IClientConfig): Promise<EppoJSClient> {


the old explicitInit method

typotter · 2025-02-05T05:16:19Z

src/i-client-config.ts

+export type IClientOptions = IApiOptions &
+  ILoggers &
+  IEventOptions &
+  IStorageOptions &
+  IPollingOptions;


The idea is that a dev could build different options objects then combine them for initialization. If they don't need custom event and storage options, for example, they wouldn't have those configs.

typotter · 2025-02-05T05:21:16Z

src/client-options-converter.ts

+  // Always include configuration request parameters
+  parameters.configurationRequestParameters = {
+    apiKey: options.sdkKey,
+    sdkVersion, // dynamically picks up version.
+    sdkName, // Hardcoded to `js-client-sdk`
+    baseUrl: options.baseUrl,
+    requestTimeoutMs: options.requestTimeoutMs,
+    numInitialRequestRetries: options.numInitialRequestRetries,
+    numPollRequestRetries: options.numPollRequestRetries,
+    pollAfterSuccessfulInitialization: options.pollAfterSuccessfulInitialization,
+    pollAfterFailedInitialization: options.pollAfterFailedInitialization,
+    pollingIntervalMs: options.pollingIntervalMs,
+    throwOnFailedInitialization: options.throwOnFailedInitialization,
+    skipInitialPoll: options.skipInitialRequest,
+  };


typotter · 2025-02-05T05:21:37Z

src/i-client-config.ts

+ * Configuration for regular client initialization
+ * @public
+ */
+export type IClientConfig = Omit<IClientOptions, 'sdkKey' | 'offline'> &


fixed up. moved sdkKey to the next major

typotter · 2025-02-05T05:21:52Z

src/index.spec.ts

+    it('should be independent of the singleton', async () => {
+      const apiOptions: IApiOptions = { sdkKey: '<MY SDK KEY>' };
+      const options: IClientOptions = { ...apiOptions, assignmentLogger: mockLogger };
+      const isolatedClient = new EppoJSClient(options);


moved to a builder

typotter · 2025-02-05T05:22:42Z

src/index.ts

+        this.readyPromiseResolver = resolve;
+      });
+    } else {
+      this.readyPromise = explicitInit(


init handles catching exceptions on init and throwing/logging depending on the option

rasendubi

Exposing constructor and init() method publicly is my only major concern.

The better handling of re-initialization is nice to have and we can implement it in future PRs (though it's a good idea to remove forceInitialize option now to avoid a breaking change in the future)

src/i-client-config.ts

rasendubi · 2025-02-05T09:26:09Z

src/index.ts

    isObfuscated: true,
  });
-  public static initialized = false;
+
+  constructor(options: EppoClientParameters) {


Shall we keep constructor private now that we use static builders?

Made the constructor private. 🥂 to progress

rasendubi · 2025-02-05T09:31:44Z

src/index.ts

+  /**
+   * Resolved when the client is initialized
+   * @private
+   */
+  private readonly initializedPromise: Promise<void>;
+
+  initialized = false;


nit: shall we move these fields (together with initializedPromiseResolver) above the constructor? It's too common to group all class fields in one fields that it becomes surprising when extra fields are "hidden" between other methods

rasendubi · 2025-02-05T09:32:35Z

src/index.ts

+
+  initialized = false;
+
+  async init(config: IClientConfig): Promise<EppoJSClient> {


Do we want to keep init private / package-only?

we do, but we can't since we need to call into it from the legacy global init method. I have marked with @internal and have tried to keep the difference between calling the global init and this class method minimal, yet with a reasonable separation of concerns. When we make a major bump, we can properly privatize this method and delete the global init.

rasendubi · 2025-02-05T09:52:43Z

src/index.ts

+      // If the instance was polling, stop.
+      this.stopPolling();
+      // Set up assignment logger and cache
+      this.setAssignmentLogger(config.assignmentLogger);


minor: again, unrelated to the PR per se and good to handle in the future — we're actually in a good position now to fix all re-initialization bugs and race conditions by re-creating a client instead of doing partial re-init.

tl;dr on the bugs: most client components are not designed to be stopped and re-started with a different configuration. In particular, the poller does not handle it well and stopPolling stops future requests but not in-progress ones, so it may still set unexpected configuration in the store. It might be fixed with some extra checks but I still wouldn't trust the client as a whole to handle re-initialization, especially because creating a new client and discarding the old one is now extremely easy

More specific proposal:

Keep client's constructor and init methods private/package-local. (Constructor is exposing internal/unsafe configuration so we don't want users calling it. init makes it too easy too shoot yourself in the foot by re-initializing the client, so I wouldn't give this gun to users either.)

Re-initialization is disallowed on client. (If constructor and init methods are private, we don't need to check for that.)

The global init function should handle forceReinitialize by creating a new singleton and stopping the old one. This is to keep backward compatibility but also because it's the only case where this makes sense.

I would argue that we should deprecate forceReinitialize in global init — if users need re-configuring the client on the fly, they are venturing into the advanced territory and using standalone clients is easier / less error-prone.

The force reinitialize is heavily used internally in tests to get around the singleton pattern, so it's super easy to get onboard with your proposal here -- deprecate and remove from the new builder

The global init function should handle forceReinitialize by creating a new singleton and stopping the old one. This is to keep backward compatibility but also because it's the only case where this makes sense.

Wouldn't creating a new instance for the singleton make any existing reference to the singleton invalid?
The initialization routine swaps out the config stores and requestor; doesn't this prevent the potential overwrite? (If both configstores are using a persistent store keyed by SDK key, they'd overwrite into the persistent store, but then that's only read on initialization if it's not expired, so there is a corner case).

came as close to your proposal as possible; stayed short of swapping out the singleton instance, but I reason that the fetchFlagConfigurations does a proper job of stopping the existing polling and swapping out the config-requesting mechanisms to grab the new request parameters.
We do still have the issue of an in-flight request being able to overwrite after it's been stopped.
I opened FF-3996 to track this as it's ultimately a bug in commons

rasendubi · 2025-02-05T10:06:12Z

src/index.ts

+  if (initializationPromise === null) {
+    initializationPromise = getInstance().init(config);


minor: again, unrelated to the PR per se but I find this initializationPromise disturbing and a potential source of errors.

If init() is called with different configuration and forceReinitialize: true, the second call is going to be silently discarded. Which may lead to all sorts of troubles as client is now running with unexpected configuration.

With forceReinitialize: false, this handling will also silence a warning, so users have less opportunity to catch their bug.

At the very least, the case of concurrent initialization is worth a warning. But overall, I think it might be a good idea to handle forceReinitialize here in this function (as per my other comment)

This "buffered" initialization was implemented because of massive amounts of traffic from one client. It is one of several measures taken to prevent the flood of requests from improper initialization of the eppo client (ex: calling init everywhere instead of getInstance).

I've added a warning here whenever init is called but the singleton instance is already initialized (or in progress).

rasendubi · 2025-02-05T10:57:02Z

src/i-client-config.ts

+  /**
+   * Force reinitialize the SDK if it is already initialized.
+   */
+  forceReinitialize?: boolean;


As per my other comment, I propose to forbid re-initialization of standalone clients (because it's buggy and it's now possible to create new clients). While we can postpone the implementation of that, I believe that buildAndInit should not accept forceReinitialize already (as removing it later will be a major change)

Agreed.
Pushed the forceReinit check to the soon-to-be-deprecated global static init method. Also moved this option to ICompatibilityOptions (maybe better named LegacyOptions?)

src/index.ts

typotter · 2025-02-14T17:11:07Z

Several false starts and iterations later, we're ready for another review. Note: this PR is now stacked on #170 where changes are made just to how the singleton is referenced, leaving the focus of this PR on waitForConfiguration and buildAndInit.

typotter added 3 commits January 30, 2025 12:13

detatched construction

e20ef73

combine new and old constructors

50e66aa

use spark for md5 instead of non-exported member from common

de9f653

typotter commented Jan 31, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

typotter changed the title ~~Tp/namespace~~ feat: non-singleton EppoJSClient Jan 31, 2025

typotter added 2 commits January 31, 2025 12:13

update to latest common, use exported members

fa5baab

doc comments

953c4e4

typotter commented Jan 31, 2025

View reviewed changes

src/i-client-config.ts Outdated Show resolved Hide resolved

typotter added 2 commits January 31, 2025 12:33

wip

159df11

skip initial config for now

a73bf0c

typotter force-pushed the tp/namespace branch from 5c466e9 to a73bf0c Compare January 31, 2025 19:35

exports

7e5abe1

typotter commented Jan 31, 2025

View reviewed changes

waitForReady

92cdd1d

typotter commented Jan 31, 2025

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

typotter commented Jan 31, 2025

View reviewed changes

clean up offline init method

c4b2717

typotter commented Jan 31, 2025

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

typotter commented Jan 31, 2025

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

typotter commented Jan 31, 2025

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

typotter commented Jan 31, 2025

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

typotter added 2 commits January 31, 2025 12:49

no need to export

ddab6c0

DOCS

d55d23d

typotter requested review from aarsilv, felipecsl and rasendubi January 31, 2025 19:54

aarsilv assigned typotter Feb 4, 2025

typotter added 2 commits February 4, 2025 15:35

builder method

3c18bc9

polish

4b8077f

typotter commented Feb 5, 2025

View reviewed changes

typotter requested review from felipecsl, aarsilv, rasendubi and sameerank February 5, 2025 05:24

rasendubi reviewed Feb 5, 2025

View reviewed changes

typotter mentioned this pull request Feb 11, 2025

chore: move initialization into class methods, limit singleton use #170

Open

typotter changed the base branch from main to typo/no-singleton February 13, 2025 20:13

typotter added 15 commits February 14, 2025 09:12

polish

6141581

test name

82ec342

move comment

81ff3b1

move forceReinit

fad43d3

docs

febb3f8

move members

212e26e

refactor wip

a943e90

chore: Move initialization code from static to the instance

ba219f2

docs

5333574

lint

aab1009

docs

f93fa61

docs

030aa33

merge fixes

e61ec6c

fix the horrible diff and merge artifacts

a000205

docs

6f1a68e

typotter commented Feb 14, 2025

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

typotter requested a review from rasendubi February 14, 2025 17:09

cleamup merge

654ffaa


		initialized = false;

		async init(config: IClientConfig): Promise<EppoJSClient> {

		if (initializationPromise === null) {
		initializationPromise = getInstance().init(config);

feat: non-singleton EppoJSClient #166

Are you sure you want to change the base?

feat: non-singleton EppoJSClient #166

Conversation

typotter commented Jan 31, 2025 • edited Loading

labels: mergeable

Motivation and Context

Description

How has this been documented?

How has this been tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typotter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rasendubi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typotter commented Feb 14, 2025

typotter commented Jan 31, 2025 •

edited

Loading